fix bug in normalize#1
Open
suborofu wants to merge 1 commit into
Open
Conversation
📝 WalkthroughWalkthroughThe ChangesNormalize function zero-guard
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
stepfg/_geometry.py (1)
14-14: ⚡ Quick winConsider using epsilon comparison for numerical stability.
The exact comparison
magnitude == 0only catches vectors with all components exactly zero. Near-zero vectors from floating-point arithmetic may have very small but non-zero magnitudes, which could lead to numerical instability when dividing. Standard practice in numerical computing is to use an epsilon threshold.💡 Suggested improvement
- if magnitude == 0: # in some cases vector_in has zeros only + if magnitude < 1e-10: # guard against zero or near-zero vectors return [0 for _ in vector_in]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@stepfg/_geometry.py` at line 14, Replace the exact zero check on magnitude with an epsilon comparison to avoid dividing by near-zero floats: instead of if magnitude == 0 use if abs(magnitude) < EPS where EPS is a small constant (e.g. 1e-12 or numpy.finfo(float).eps) defined near the top of the module; when the magnitude is below EPS handle the near-zero vector_in the same way you currently handle exact zeros (return a zero vector or skip normalization) to preserve numerical stability. Ensure you reference the same symbols (magnitude, vector_in) and add an import or constant definition for EPS if not already present.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@stepfg/_geometry.py`:
- Around line 14-15: The normalize() routine currently returns [0,0,0] for zero
magnitude which propagates into STEP DIRECTION/_axis2_placement_3d; change
behavior to either (a) make normalize(vector) raise a clear ValueError on zero
magnitude (update function normalize in stepfg._geometry) or (b) keep normalize
but add explicit fallbacks at callers in stepfg._builder where
normalize(direction), normalize(zaxis), normalize(vertices[i] - vertices[0]) and
normalize(cross_product(...)) are used: detect zero magnitude and substitute a
well-defined default unit axis (e.g., [1,0,0] or compute a non-parallel axis) or
recompute a valid orthonormal, and add a unit test exercising normalization of a
zero vector to ensure the resolver path is exercised; pick one approach and
apply consistently across the referenced call sites and tests.
---
Nitpick comments:
In `@stepfg/_geometry.py`:
- Line 14: Replace the exact zero check on magnitude with an epsilon comparison
to avoid dividing by near-zero floats: instead of if magnitude == 0 use if
abs(magnitude) < EPS where EPS is a small constant (e.g. 1e-12 or
numpy.finfo(float).eps) defined near the top of the module; when the magnitude
is below EPS handle the near-zero vector_in the same way you currently handle
exact zeros (return a zero vector or skip normalization) to preserve numerical
stability. Ensure you reference the same symbols (magnitude, vector_in) and add
an import or constant definition for EPS if not already present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+14
to
+15
| if magnitude == 0: # in some cases vector_in has zeros only | ||
| return [0 for _ in vector_in] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find all callers of normalize() to verify they handle zero vector returns
# Find all calls to normalize()
rg -n -B3 -A3 'normalize\(' --type=py -g '!test_*.py' -g '!*_test.py'Repository: evvaletov/stepfg
Length of output: 2850
Fix zero-vector handling for normalize() results used as STEP directions
stepfg/_builder.pypassesnormalize(...)results directly into STEPDIRECTIONand_axis2_placement_3dinputs (e.g.,normalize(direction);normalize(zaxis)/normalize(vertices[*] - vertices[0]); andnormalize(cross_product(...))), so a[0, 0, 0]return would propagate into emitted geometry.- The check
if list(map(operator.add, normalize(zaxis), normalize(cp))) == [0, 0, 0]:only flips winding; it does not prevent emitting[0, 0, 0]direction vectors.
Handle zero magnitude either by making normalize() raise (or otherwise refuse), or by adding a well-defined fallback at these call sites, and add a test covering zero-vector normalization.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@stepfg/_geometry.py` around lines 14 - 15, The normalize() routine currently
returns [0,0,0] for zero magnitude which propagates into STEP
DIRECTION/_axis2_placement_3d; change behavior to either (a) make
normalize(vector) raise a clear ValueError on zero magnitude (update function
normalize in stepfg._geometry) or (b) keep normalize but add explicit fallbacks
at callers in stepfg._builder where normalize(direction), normalize(zaxis),
normalize(vertices[i] - vertices[0]) and normalize(cross_product(...)) are used:
detect zero magnitude and substitute a well-defined default unit axis (e.g.,
[1,0,0] or compute a non-parallel axis) or recompute a valid orthonormal, and
add a unit test exercising normalization of a zero vector to ensure the resolver
path is exercised; pick one approach and apply consistently across the
referenced call sites and tests.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit